Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use /run as the default for PID/UNIX-domain sockets #13

Merged
merged 6 commits into from
Aug 7, 2024
Merged

Use /run as the default for PID/UNIX-domain sockets #13

merged 6 commits into from
Aug 7, 2024

Conversation

omus
Copy link
Member

@omus omus commented Aug 7, 2024

While reviewing #12 I needed to review when UNIX-domain sockets or named pipes were created. The answer seems to be UNIX-domain sockets are created on Linux and macOS and only Windows creates named pipes. I also reviewed the filesystem hierarchy standard (FHS) which states that /run should store both PID files and transient UNIX-domain sockets and realized that the /run directory could be made writable on read-only file systems with mounts which eliminated the need for DEPUTY_IPC_DIR.

Unfortunately, in order to allow for local testing of K8sDeputy.jl we need to keep DEPUTY_IPC_DIR around as normally /run is not writable by a normal user.

Supersedes #12.

test/graceful_termination.jl Outdated Show resolved Hide resolved
test/health.jl Outdated Show resolved Hide resolved
test/health.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (973acac) to head (18cec00).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #13      +/-   ##
==========================================
- Coverage   90.12%   89.61%   -0.52%     
==========================================
  Files           4        4              
  Lines          81       77       -4     
==========================================
- Hits           73       69       -4     
  Misses          8        8              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@omus omus requested a review from kleinschmidt August 7, 2024 18:54
function _graceful_terminator_socket_path(pid::Int32)
name = "graceful-terminator.$pid"
return haskey(ENV, "DEPUTY_IPC_DIR") ? joinpath(_deputy_ipc_dir(), name) : name
return joinpath(_deputy_ipc_dir(), "graceful-terminator.$pid.socket")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could mock this call to the IPC dir for testing purposes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also possibly use mocking for the external command. Will look into that quickly.

@@ -12,6 +14,7 @@
end

cmd = `$(Base.julia_cmd()) --color=no -e $code`
cmd = addenv(cmd, "DEPUTY_IPC_DIR" => deputy_ipc_dir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah maybe not if we are using an external command...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the very least it seems that it woudl be annoying lol

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely annoying. I'll look into replacing DEPUTY_IPC_DIR with Mocking as a follow up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah let's not hold this up

@omus omus merged commit b62e185 into main Aug 7, 2024
6 of 7 checks passed
@omus omus deleted the cv/run branch August 7, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants